Skip to content

*: Deterministic error work in preparation for gas#2112

Merged
That3Percent merged 1 commit into
masterfrom
deterministic-errors
Jan 22, 2021
Merged

*: Deterministic error work in preparation for gas#2112
That3Percent merged 1 commit into
masterfrom
deterministic-errors

Conversation

@That3Percent
Copy link
Copy Markdown
Contributor

@That3Percent That3Percent commented Jan 14, 2021

Error Determinism

  • Introduced determinism in error types
  • Before this would set deterministic_host_trap, but access to this value may not be available in many cases where the error originates or may use shared references forcing atomics
  • anyhow errors are necessarily considered non-deterministic until proven otherwise.
  • Anyhow is dangerous for our use-case because it hides information by design. (This extends to any use-case which handles errors) There's not much that can be done here in the short term.
  • Lots of anyhow errors have been converted to deterministic ones (eg: json parsing, type conversions, math, and most other host exports are deterministic)
  • Note that like anyhow, panics do not carry determinism information. These must be treated as non-deterministic and should be avoided for error conditions arising out of the subgraph or the data it acts upon.

Non-Panicky Errors

  • BigDecimal % 0 no longer panics but has DeterministicHostError instead
  • Before the wasm was trusted to interact with the heap in-bounds, leading to panics - which with this change is a DeterministicHostError instead
  • Same story for assertions on binary representations for AscValue which also are untrusted inputs
    There's probably a lot more that can be done along these lines...

@That3Percent That3Percent requested a review from leoyvens January 14, 2021 20:33
leoyvens
leoyvens reviewed Jan 21, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@leoyvens leoyvens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very happy to see this lift done!

Comment thread graph/src/components/subgraph/host.rs
Comment thread runtime/derive/src/lib.rs Outdated
Comment thread runtime/wasm/src/asc_abi/class.rs Outdated
// initially at the start of the content skipping `length`.
let mut offset = size_of::<u32>();

// TODO: Verify that the length specified matches the byte length
Copy link
Copy Markdown
Collaborator

@leoyvens leoyvens Jan 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the situation with this TODO?

Copy link
Copy Markdown
Contributor Author

@That3Percent That3Percent Jan 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code as it was before (and as it is now) does not verify that the value of the first 4 bytes specifying the string length coincides with the length of asc_obj.

Copy link
Copy Markdown
Collaborator

@leoyvens leoyvens Jan 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah got it

Comment thread runtime/wasm/src/asc_abi/class.rs
Comment thread runtime/wasm/src/asc_abi/mod.rs
Comment thread runtime/wasm/src/module/mod.rs
Comment thread runtime/wasm/src/module/mod.rs
link_ptr: AscPtr<AscString>,
) -> Result<AscPtr<Uint8Array>, HostExportError> {
if !self.experimental_features.allow_non_deterministic_ipfs {
return Err(HostExportError::Deterministic(anyhow!(
Copy link
Copy Markdown
Collaborator

@leoyvens leoyvens Jan 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't consider this deterministic because it depends on an env var set by the indexer.

Copy link
Copy Markdown
Contributor Author

@That3Percent That3Percent Jan 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spent a while waffling about how to handle this. We've used the word "determinism" to actually mean multiple conflated things in different contexts. In this case, the error is in fact attestable. Within the context of The Network this subgraph should always fail with this error, deterministically. I'd be interested in a longer chat on slack about whether or not we should tease these apart somehow or what.

Copy link
Copy Markdown
Collaborator

@leoyvens leoyvens Jan 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point that in the network the this should never be allowed so it is deterministic. Ok we can keep this as deterministic.

Comment thread runtime/wasm/src/module/mod.rs
Comment thread runtime/wasm/src/module/mod.rs
Comment thread runtime/wasm/src/module/mod.rs
@That3Percent That3Percent merged commit df8e384 into master Jan 22, 2021
@That3Percent That3Percent deleted the deterministic-errors branch January 22, 2021 21:26
This was referenced Jan 28, 2021
Closed
Closed
@enzojavier-desimone
Copy link
Copy Markdown

enzojavier-desimone commented Mar 9, 2022

I think that the issue that I am facing is related to this issue,

I am running in local the ens-subgraph when reaching to the first block to index I have the error

Mar 08 18:28:07.005 ERRO Subgraph error 1/1, code: SubgraphSyncingFailure, error: `ens_name_by_hash` is deprecated	wasm backtrace:	    0: 0x2e77 - <unknown>!src/ensRegistry/handleNewOwner	 in handler `handleNewOwnerOldRegistry` at block #3330220 (4ea8feee666e509db9d574b097efaa47b23de6b06fb33222dbed1484b1ca4748), block_hash: 0x4ea8feee666e509db9d574b097efaa47b23de6b06fb33222dbed1484b1ca4748, block_number: 3330220, sgd: 1, subgraph_id: QmYbUu3ZfmrK8ugavLFEnz2hHHiEUx22ya3SwqwBe8HuKQ, component: SubgraphInstanceManager
Mar 08 18:28:07.041 ERRO Subgraph instance failed to run: `ens_name_by_hash` is deprecated	wasm backtrace:	    0: 0x2e77 - <unknown>!src/ensRegistry/handleNewOwner	 in handler `handleNewOwnerOldRegistry` at block #3330220 (4ea8feee666e509db9d574b097efaa47b23de6b06fb33222dbed1484b1ca4748), code: SubgraphSyncingFailure, sgd: 1, subgraph_id: QmYbUu3ZfmrK8ugavLFEnz2hHHiEUx22ya3SwqwBe8HuKQ, component: SubgraphInstanceManager

doing some research on it I see that it comes for this implementation here https://github.com/graphprotocol/graph-node/blob/master/runtime/wasm/src/module/mod.rs#L1576

then my question is, how it is possible to set the experimental_features? I would like to change the value of allow_non_deterministic_ipfs but I could not find a command-line param or a env variable for it

@enzojavier-desimone enzojavier-desimone mentioned this pull request Mar 9, 2022
Merged
@enzojavier-desimone
Copy link
Copy Markdown

enzojavier-desimone commented Mar 9, 2022 •
edited
Loading

UPDATE, I found the undocumented env variable for set the experimental feature GRAPH_ALLOW_NON_DETERMINISTIC_IPFS which is missing in the doc https://github.com/graphprotocol/graph-node/blob/master/docs/environment-variables.md

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@leoyvens leoyvens leoyvens approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@That3Percent @enzojavier-desimone @leoyvens